-
Notifications
You must be signed in to change notification settings - Fork 8
fix(mongodb-runner): make close operation more resilient #571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Store identifying information about a server in a database, then only actually run `process.kill()` on servers where this identifying information matches what we expect it to match. Also, do not kill processes to which we cannot connect, and instead assume they have already been closed before. Fixes: #483
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, 1 Q.
Hoping this will help with some flakiness or at least make the tests fail quicker
ah on mongosh end, this will mainly affect smoke tests
const client = await MongoClient.connect( | ||
`mongodb://${this.hostport}/?directConnection=true`, | ||
// directConnection + retryWrites let us write to `local` db on secondaries | ||
`mongodb://${this.hostport}/?directConnection=true&retryWrites=false`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can retryWrites
end up messing with our tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm ... how would it do that? I can't think of a place where we would even make use of this client. It is a public API, to be fair, but where we do use the withClient()
feature, I'm pretty sure we do that on the MongoCluster
level, not the MongoServer
level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah, I was thinking that we were interfacing with it through this client somehow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Store identifying information about a server in a database, then only actually run
process.kill()
on servers where this identifying information matches what we expect it to match.Also, do not kill processes to which we cannot connect, and instead assume they have already been closed before.
Fixes: #483
Description
Open Questions
Checklist